-
-
Notifications
You must be signed in to change notification settings - Fork 484
feat: Add support for Guild Scheduled Event Recurrence #2749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: plun1331 <plun1331@gmail.com> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
… into feat/recurrence-rule
discord/scheduled_events.py
Outdated
| If this recurrence rule was obtained from the API you will need to | ||
| :meth:`.copy` it in order to edit it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more question... why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class seems to serve more as a dataclass-ish than an API object. Which is fine, but having an edit method here, and then disallowing editing attributes seems weird. This class should realistically not even hold self._state (I believe). It would probably make everything easier.
|
Merge conflicts |
b55c125 to
82659b2
Compare
|
@DA-344 went ahead and fixed the merge conflicts for you. please double check |
discord/scheduled_events.py
Outdated
| If this recurrence rule was obtained from the API you will need to | ||
| :meth:`.copy` it in order to edit it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this
| image: Optional[:class:`bytes`] | ||
| The cover image of the scheduled event | ||
| The cover image of the scheduled event. | ||
| recurrence_rule: Optional[:class:`ScheduledEventRecurrenceRule`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is provided, is start_date ignored ?
Signed-off-by: Lumouille <144063653+Lumabots@users.noreply.github.com>
Signed-off-by: Lala Sabathil <lala@pycord.dev>
Paillat-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see merge conflicts
| return f"<ScheduledEventRecurrenceRule start_date={self.start_date} frequency={self.frequency} interval={self.interval}>" | ||
|
|
||
| @property | ||
| def weekdays(self) -> list[ScheduledEventWeekday]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this again. If you can, I think we should just make these normal attributes. Realistically, it's more likely to confuse someone than prevent misuse. See my comment on edit below as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that these should be properties so when users set a new value (rrule.weekdays = [...]) the required processing and checks can be done so the payload sent when using _to_dict() is correctly formatted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of there properties process anything, all they do is returning a copy (which again i don't think we should do, to be honest with you as I mentioned somewhere else this could have been a dataclass), and if there's any processing to be done it can be done in _to_dict directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant on the setters lol.
but now they do not return copies, i don't remember in which commit i changed that (in which i forgot to update the docstring)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I just checked the docstring.
discord/scheduled_events.py
Outdated
| If this recurrence rule was obtained from the API you will need to | ||
| :meth:`.copy` it in order to edit it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class seems to serve more as a dataclass-ish than an API object. Which is fine, but having an edit method here, and then disallowing editing attributes seems weird. This class should realistically not even hold self._state (I believe). It would probably make everything easier.
Co-authored-by: Paillat <paillat@pycord.dev> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
discord/scheduled_events.py
Outdated
| self.exceptions: list[Object] = list( | ||
| map( | ||
| Object, | ||
| data.get("guild_scheduled_events_exceptions") or [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i cant find any docs on guild_scheduled_events_exceptions, could you link one to me ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well so its not documented by discord it shouldnt be here imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this before exceptions were changed, but yeah, I will remove them (I think i already did, or maybe i just didn't push the commit)
| *, | ||
| weekdays: list[WeekDay | ScheduledEventWeekday] = MISSING, | ||
| n_weekdays: list[NWeekDay] = MISSING, | ||
| month_days: list[datetime.date] = MISSING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work with two different days on two different months ? Realistically that would end up to 4x a year instead of 2x a year when casted into discord's api's format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would end up with 2 pairs of days on by_month and by_month_day, maybe adding a note saying that the API currently supports "1 month day".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo this should just accept a list of ints, it would just be clearer and more importantly closer to the Discord API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can make it so it can take both datetime.date or a sequence of tuple of ints (with a structure like (month, day)), or just change to a format, although the latter seems like a worse ux design when we can use built-in dataclasses specifically made for representing month days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like having both would be confusing (in general, we are trying to avoid adding many ways of doing things, this is a recurring issue in py-cord), and having only datetime.date isn't correctly representative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @plun1331 do you have an idea ?
… into feat/recurrence-rule
| daily = 3 | ||
|
|
||
|
|
||
| class ScheduledEventWeekday(Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weekday implies the days of the typical working week Monday through Friday. Maybe we can use DayOfWeek?
docs/api/enums.rst
Outdated
|
|
||
| .. attribute:: monday | ||
|
|
||
| Monday, the first day of the week. Index of 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to get political, the first day of the week is not internationally recognized as Monday. Personally, I think we can just drop "the [x] day of the week" from the documentation, as a reasonable individual should know what a Monday is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that makes sense, will do.
|
Not wanting to be disrespectful or anything, but it’s been some time since this hasn’t been updated or fixed as it’s implementing, if I understand correctly, a wrong/limited usage of schedule event (see Paillat comment). |
Co-authored-by: JustaSqu1d <89910983+JustaSqu1d@users.noreply.github.com> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
Co-authored-by: JustaSqu1d <89910983+JustaSqu1d@users.noreply.github.com> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
Summary
Adds support for receiving, setting, and updating a Scheduled Event's recurrence rule.
Documentation: resources/guild-scheduled-event
Needs testing.
Information
examples, ...).
Checklist
type: ignorecomments were used, a comment is also left explaining why.